Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make config raise specific domain specific error #176

Merged
merged 2 commits into from
Oct 27, 2022
Merged

Conversation

telphan
Copy link
Contributor

@telphan telphan commented Oct 27, 2022

The motivation for this PR is to be able to target these errors specifically.

@telphan telphan force-pushed the new_argument_error branch from 3abe19e to c77645f Compare October 27, 2022 12:32
@telphan
Copy link
Contributor Author

telphan commented Oct 27, 2022

We need to disable the requirement for:
Elixir 1.7 / OTP 23.0
Elixir 1.8 / OTP 23.0
Elixir 1.9 / OTP 23.0

As these are no longer supported

@telphan telphan merged commit 2202dee into main Oct 27, 2022
@telphan telphan deleted the new_argument_error branch October 27, 2022 13:33
@@ -20,6 +20,10 @@ defmodule Paginator.Config do
:total_count_limit
]

defmodule ArgumentError do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: I don't think this is necessary since the Elixir stdlib for Exceptions already has this [1]? @ulissesalmeida: Is that correct?

[1] https://github.com/elixir-lang/elixir/blob/main/lib/elixir/lib/exception.ex#L807

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yea, we should use def exception indeed 👍🏽

jesse-c pushed a commit that referenced this pull request Oct 27, 2022
This is to test my idea from
#176 (comment)
where I've said that I don't think this is necessary since the Elixir
stdlib for Exceptions already has this [1]?

[1] https://github.com/elixir-lang/elixir/blob/main/lib/elixir/lib/exception.ex#L807
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants